Lowercaseify Nexus request headers#1006
Conversation
| }; | ||
|
|
||
| if let Some(ref mut req) = t.resp.request { | ||
| req.header = normalize_http_headers(std::mem::take(&mut req.header)); |
There was a problem hiding this comment.
why do we std::mem::take here? Just for safety, to make sure that memory is cleared out?
There was a problem hiding this comment.
Can we just accept a mutable hash map reference in the normalize helper?
There was a problem hiding this comment.
It allows me to own the old map, therefore I can call into_iter and avoid copying all the values
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
(sorry, forgot to publish this review, too late now)
| }; | ||
|
|
||
| if let Some(ref mut req) = t.resp.request { | ||
| req.header = normalize_http_headers(std::mem::take(&mut req.header)); |
There was a problem hiding this comment.
Can we just accept a mutable hash map reference in the normalize helper?
| /// Given a header map, lowercase all the keys and return it as a new map. | ||
| /// Any keys that are duplicated after lowercasing will clobber each other in underfined ordering. | ||
| pub fn normalize_http_headers(headers: HashMap<String, String>) -> HashMap<String, String> { | ||
| let mut new_headers = HashMap::new(); |
There was a problem hiding this comment.
I think it'd be better to accept a mutable map if we can, but if we must do it this way, would recommend either using an iter approach (HashMap's FromIterator just drops dupes) or at least using with_capacity instead of new.
There was a problem hiding this comment.
You cannot mutate hashmap keys. with_capacity, sure, but, it's lightyears away from being a bit of perf that matters.
3 participants
What was changed
Lowercaseify Nexus start request headers
Why?
So lang doesn't see weird duplicated camel and kebab case of the same header.
Checklist
Closes [Feature Request] Nexus should deduplicate and lowercase inbound headers #993
How was this tested:
Existing integ test with modification
Any docs updates needed?